Skip to content

[WD-22062] Make non-webpage nodes unclickable and exclude partials #200

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 11 commits into from
Jun 23, 2025
Merged

[WD-22062] Make non-webpage nodes unclickable and exclude partials #200

merged 11 commits into from
Jun 23, 2025

Conversation

@muhammad-ali-pk
Copy link
Contributor

@muhammad-ali-pk muhammad-ali-pk commented Jun 4, 2025 •
edited
Loading

Note: Revert this temporary QA change before merging

# TODO: Revert this line to `if not webpages ...` before merging this PR
# This is only for QA
if True or not webpages or self._has_incomplete_pages(webpages):

Done

  • Exclude partials and manually provided excluded directories from tree generation
  • Store node extension e.g., .html, .md, .dir
  • Disable non-webpage nodes in sidebar tree so they are not clickable
  • Disable breadcrumbs for non-webpage url paths so they are not clickable
  • Hide non-webpage nodes from table view
  • [drive by]: modify the findPage helper to conditionally return bool or webpage object
  • Added tooltip on disabled parts of the breadcrumb

QA

QA steps

  • Clone this repo and check out this PR
  • Run the project using
rm -rf .venv
docker run -d -p 5432:5432 -e POSTGRES_PASSWORD=postgres postgre
docker run -d -p 6379:6379
python -m venv .venv
source .venv/bin/activate
python -m pip install -r requirements.txt
set -a
source .env
source .env.local
set +a
flask --app webapp.app run --debug

In a separate terminal, run

yarn dev

In a separate terminal, start the celery worker

source .venv/bin/activate
set -a
source .env
source .env.local
set +a

celery -A webapp.app.celery_app worker -B  --loglevel=DEBUG
  • The application should be accessible at localhost:5000 and should show loading when cloning repositories
  • In the left sidebar, select TREE VIEW and choose canonical.com
  • Find "legal" and click on it
  • It should only show the nested children and not open up a detailed page
  • Click on the nested children, if they are actual webpages they will be accessible, else if they are only intermediary directories without actual webpage, they will not be clickable.
  • Click legal > contributors > faq, the detailed page should open up for faq page. Verify the breadcrumbs are disabled for non-webpage paths.
  • Parent pages (folders) that are not webpages should not be clickable. Pages that are nodes (not parent pages) that are not webpages (like partials) should not be returned by “get-tree” endpoint.

Fixes

Screenshots

simplescreenrecorder-2025-06-04_10 10 23-ezgif com-video-to-gif-converter

@webteam-app
Copy link

Demo

Jenkins

demos.haus

@petesfrench petesfrench self-requested a review June 5, 2025 06:54
@petesfrench petesfrench self-assigned this Jun 5, 2025
@petesfrench
Copy link

petesfrench commented Jun 5, 2025 •
edited
Loading

  • Nested folders that are web pages are not clickable in the tree view or in breadcrumbs, for example appliance > adguard (should be clickable) > intel-nuc

image

  • For parent pages (folders) that are not webpages, there should probably be some kind of visual indicator to show the user that it will behave differently to avoid any confusion. My suggestion would be to put them in italics, but we should probably get input from UX.

  • Out of scope: There is no keyboard navigation support for the tree view. Would be worth keeping this in mind early as it will a lot harder to implement retroactively.

@muhammad-ali-pk
Copy link
Contributor Author

muhammad-ali-pk commented Jun 10, 2025 •
edited
Loading

@petesfrench, thanks for the review.

Nested folders that are web pages are not clickable in the tree view or in breadcrumbs, for example appliance > adguard (should be clickable) > intel-nuc

We are not yet parsing markdown pages (like adguard for example). So for all intents and purposes, it's not considered a valid webpage at the moments. We will be adding support for markdown pages later, as request in this ticket.

For parent pages (folders) that are not webpages, there should probably be some kind of visual indicator to show the user that it will behave differently to avoid any confusion. My suggestion would be to put them in italics, but we should probably get input from UX.

Although there isn't a UX task for it, @eliman11 could you give us a quick suggestion on the visual hint for webpages which are not clickable and purely serve the purpose of fulfilling the path? For example, the breadcrumbs in ubuntu.com>appliance>adguard>intel-nuc, only ubuntu.com and appliance are clickable and act as links, and adguard is just text at the moment (the last part is always text and default, because it's the ending node)

petesfrench
petesfrench reviewed Jun 10, 2025
View reviewed changes
@eliman11
Copy link

Although there isn't a UX task for it, @eliman11 could you give us a quick suggestion on the visual hint for webpages which are not clickable and purely serve the purpose of fulfilling the path? For example, the breadcrumbs in ubuntu.com>appliance>adguard>intel-nuc, only ubuntu.com and appliance are clickable and act as links, and adguard is just text at the moment (the last part is always text and default, because it's the ending node)

Hi, sorry if I'm missing something - is adguard not a page on our website (https://ubuntu.com/appliance/adguard)? Is there a reason why we don't have it on the content system?

@muhammad-ali-pk
Copy link
Contributor Author

@eliman11 Adguard is indeed a page, and you can also find it on the content system. It's actually a markdown page (.md) and currently, when parsing page details like title, description etc., we are not parsing it for markdown pages (we will be soon though). There will be some pages in content system, that are not a webpage, but will just be intermediate paths for webpages.
The thing we need your help with is that sometimes, a path will itself be not a webpage, but an intermediate part to path of a webpage. For example, in domain.com/intermediate/webpage, "intermediate" might not be a webpage and we have no details to show for it, so we want to disable click on this path. For this, we will not be showing it as a link in the breadcrumbs, rather just a plain text. Pete and I were wondering if there can be other ways to show this difference, like italic text perhaps?

image

@eliman11
Copy link

Got it! In case we have that, could we use the disabled base button state (here) - we don't have it as a variant of the breadcrumb but I think it indeed makes sense here. It's essentially the black default colour with 33% opacity. Could we also have the tooltip available on hover to clarify why this part of the path is disabled? Let me know if this works!
Screenshot 2025-06-13 at 09 35 23

@muhammad-ali-pk
Copy link
Contributor Author

muhammad-ali-pk commented Jun 17, 2025 •
edited
Loading

@eliman11 Definitely!

image

petesfrench
petesfrench approved these changes Jun 19, 2025
View reviewed changes
Copy link

@petesfrench petesfrench left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@muhammad-ali-pk muhammad-ali-pk merged commit eeaf097 into main Jun 23, 2025
7 of 8 checks passed
@muhammad-ali-pk muhammad-ali-pk deleted the WD-22062 branch June 23, 2025 14:11
@github-actions
Copy link

github-actions bot commented Sep 5, 2025

🎉 This PR is included in version 1.0.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@petesfrench petesfrench petesfrench approved these changes

Assignees

@petesfrench petesfrench

Labels

released Review: Code +1 Review: QA +1

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@muhammad-ali-pk @webteam-app @petesfrench @eliman11